Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mount SD card as RW on kobo, but ignore errors #1845

Merged
merged 2 commits into from
Feb 24, 2016
Merged

Conversation

Hzj-jie
Copy link
Contributor

@Hzj-jie Hzj-jie commented Feb 22, 2016

This change is to mount sd card as RW on kobo devices. Currently we are using sdr doc config, which needs write permission to the location where the original books store. But by default, sd card on kobo is mounted as RO, so the sdr doc config won't be written.

@@ -80,6 +80,9 @@ if [ ! -n "${PLATFORM}" ] ; then
fi
# end of value check of PLATFORM

# Remount SD to RW, ignore errors since we may not have sd card
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of ignoring errors no matter what wouldn't it be better to perform a test like if [ -f /mnt/sd ]; then or mount | grep /tmp/sd (exit status is 0 or is not 1) or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If [ -f won't work, the /mnt/sd folder always exists, even sd card is not mounted. And one more concern is, even /dev/mmcblk1p1 (sd card block device) is existing, we may still not able to mount it as RW. i.e. the file system may not be supported. But if the file system is supported, remount as RW should always succeed. So simply ignoring the error may be the most simply way to solve most cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about my second suggestion regarding checking whether it is in fact already mounted using either mount itself or e.g. cat /proc/mounts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I surely can implement a more complex logic, i.e.

mount | grep -P ' /mnt/sd .*rw'
if [ $? ]
then
mount -o remount,rw /mnt/sd
fi

But it seems the change won't make things better. We may still fail to mount the partition as RW, which has exactly same possibility as current solution.

What do you think?

@Frenzie
Copy link
Member

Frenzie commented Feb 22, 2016

I'm inclined to think that a failed to remount error is at least
potentially more useful when it doesn't include SD card not being present
at all errors.
On Feb 22, 2016 12:46 PM, "Hzj_jie" notifications@github.com wrote:

I surely can implement a more complex logic, i.e.

mount | grep -P ' /mnt/sd .*rw'
if [ $? ]
then
mount -o remount,rw /mnt/sd
fi

But it seems the change won't make things better. We may still fail to
mount the partition as RW, which has exactly same possibility as current
solution.

What do you think?


Reply to this email directly or view it on GitHub
#1845 (comment).

@Hzj-jie
Copy link
Contributor Author

Hzj-jie commented Feb 22, 2016

Frenzie, there must be something I have not described clearly.
By default, the SD card is mounted as RO, i.e. koreader can access it, read without trouble. And the history information is also updated, since we write them in both history folder and sdr folder. Even sdr folder is not writable, there is no error message at all. This is a critical issue to block us from migrating history to sdr folder. So this change does not want to mount the SD card, but remount it as RW. So if mount command failed, but indeed SD card is presenting, things won't become worse. But if mount command succeeded, extra check won't make things better (users may not have sd card installed on their kobos), in such cases, 'check and do nothing' won't be better than 'try to do it, but fail to do so'.
Do you prefer to have some log about mount result, or do not start reader.lua if remount failed?

KenMaltby, are you sure it may fail on your device? Actually this command is exactly the same as what's in KSM. P.S. I am also using a Kobo AuraHD.

@KenMaltby
Copy link

Tshering's scripts to remount and check the status of the external SD include feedback to the user; including:

answer=$(mount -v -o remount,rw /mnt/sd 2>&1)
case $? in
0 ) echo "remounted as read-write";;

  • ) echo "$answer";;
    esac

I don't know why I had to insure the trailing "/" was there, to get it to work in recent implementations, but that is my experience.

@NiLuJe
Copy link
Member

NiLuJe commented Feb 22, 2016

Look at the platform/kindle/koreader.sh script if you want an example of cheap "is something already mounted?" check, where we use a bunch of them when bind-mounting custom font paths.

@Frenzie
Copy link
Member

Frenzie commented Feb 22, 2016

@Hzj-jie

But if mount command succeeded, extra check won't make things better (users may not have sd card installed on their kobos), in such cases, 'check and do nothing' won't be better than 'try to do it, but fail to do so'.

I think it would make for better debugging by not spamming error messages about an SD card that isn't there in the first place.

Do you prefer to have some log about mount result, or do not start reader.lua if remount failed?

As a user I'd rather have it working imperfectly than being faced with a draconian denial of service.

@houqp
Copy link
Member

houqp commented Feb 23, 2016

OK, based on the discussion above, I think it can be summarized as below.

  • no sd card: no /mnt/sd/ found in /proc/mounts, simply ignore and do nothing
  • has sd card: found /mnt/sd in /proc/mounts
    • /mnt/sd listed as RW in /proc/mounts. we are good, proceed and do nothing
    • /mnt/sd listed as RO in /proc/mounts. Try remount as RW. In case of failure, log the error, but keep bootstrapping the reader.

Let me know if I missed anything ;)

@Hzj-jie is right in that simply doing mount -o remount,rw /mnt/sd || true without all the extra condition checks will have the same result. i.e. it will only work if you have a properly formatted SD card.

That said, I too prefer we put in some extra checking logic here for couple reasons:

  • to avoid unnecessary system calls. If the mount system call can be avoided with a read, why not do a grep first ;P Yes, it won't change the end result, but it's less expensive and it feels cleaner.
  • it's only couple extra lines of shell commands, so trivia to do.
  • keep the error handling style consistent with what @NiLuJe did in the kindle script
  • it's harmless and won't break anything

Lastly, thank you @Hzj-jie for working on this legacy setting cleanup effort :)

@KenMaltby
Copy link

While I have been using a remount of the external uSD card, without checks for a long time now and have had no problems related to that, it will be nice not to have to add it with each version update. I think the checks are needed, as you can never tell what the average user may encounter thru mischance or accident.

@Hzj-jie
Copy link
Contributor Author

Hzj-jie commented Feb 23, 2016

KenMaltby, no, no matter existing solution (with check) or older solution (without check), end users won't be impacted if sd card is not mountable or not existing.

I have updated this change with a check. Please kindly let me know if we still prefer '|| true' for remount command.

@NiLuJe
Copy link
Member

NiLuJe commented Feb 23, 2016

If you need battle-tested 'is something mounted ro?' and 'remount that rw!' checks, we have some in MRPI. It might be a tad overkill, but I'm not as familiar with how the various Kobo might mess this up to judge ;).

I can just say that something as simple as that bit us in the ass a couple of times until it was reimplemented like this on the Kindle side ;).

@KenMaltby
Copy link

For the Kobos there is also the use of "Launchers", like KSM, that might be used to change the status of the external drive and of course there is Nickel and its impact on things. Also "syncthing", now.

http://www.mobileread.com/forums/showpost.php?p=3266630&postcount=122
http://www.mobileread.com/forums/showpost.php?p=3266654&postcount=123

@Hzj-jie
Copy link
Contributor Author

Hzj-jie commented Feb 23, 2016

Yes, I have a kobo aura hd myself. The machine always starts with sd card mounted as ro, so I prepared this change to make koreader sdr config work for sd card.

houqp added a commit that referenced this pull request Feb 24, 2016
Mount SD card as RW on kobo, but ignore errors
@houqp houqp merged commit abd4727 into koreader:master Feb 24, 2016
@houqp
Copy link
Member

houqp commented Feb 24, 2016

Thanks @NiLuJe for the pointer to MRPI, it does looks more robust. We should definitely do the same for kroeader too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants